fix(filesystem): only change folder permissions if required

On Linux changing the permissions causes inotify to create a IN_ATTRIB
event -- even if the permissions stays the same.

Such an event is passed to the filesystem watcher which lets the client
schedule a new sync run.  In certain conditions, this could happen
during every sync run...

Signed-off-by: Jyrki Gadinger <nilsding@nilsding.org>

diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp
index 5603e4f4eaa384769155915faf3350d86ceee572..3b06ff1d72fe00e8ea2cbd143409b75cad699288 100644 (file)
--- a/src/libsync/filesystem.cpp
+++ b/src/libsync/filesystem.cpp
@@ -353,8 +353,15 @@ bool FileSystem::getInode(const QString &filename, quint64 *inode)
 }
 
 bool FileSystem::setFolderPermissions(const QString &path,
-                                      FileSystem::FolderPermissions permissions) noexcept
+                                      FileSystem::FolderPermissions permissions,
+                                      bool * const permissionsChanged) noexcept
 {
+    bool permissionsDidChange = false;
+
+    if (permissionsChanged) {
+        *permissionsChanged = false;
+    }
+
 #ifdef Q_OS_WIN
     SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION;
     std::unique_ptr<char[]> securityDescriptor;
@@ -494,18 +501,47 @@ bool FileSystem::setFolderPermissions(const QString &path,
         qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << QDir::toNativeSeparators(path) << GetLastError();
         return false;
     }
+
+    permissionsDidChange = true;
 #else
     static constexpr auto writePerms = std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write;
     const auto stdStrPath = path.toStdWString();
+
+    const auto currentPermissions = std::filesystem::status(stdStrPath).permissions();
+    qCDebug(lcFileSystem()).nospace() << "current permissions path=" << path << " perms=" << Qt::showbase << Qt::oct << static_cast<int>(currentPermissions);
+
     try
     {
         switch (permissions) {
-        case OCC::FileSystem::FolderPermissions::ReadOnly:
-            std::filesystem::permissions(stdStrPath, writePerms, std::filesystem::perm_options::remove);
+        case OCC::FileSystem::FolderPermissions::ReadOnly: {
+            qCDebug(lcFileSystem()).nospace() << "ensuring folder is read only path=" << path;
+
+            if ((currentPermissions & writePerms) != std::filesystem::perms::none) {
+                qCDebug(lcFileSystem()).nospace() << "removing owner/group/others write permissions path=" << path;
+                std::filesystem::permissions(stdStrPath, writePerms, std::filesystem::perm_options::remove);
+                permissionsDidChange = true;
+            }
+
             break;
-        case OCC::FileSystem::FolderPermissions::ReadWrite:
+        }
+        case OCC::FileSystem::FolderPermissions::ReadWrite: {
+            qCDebug(lcFileSystem()).nospace() << "ensuring folder is read/writable path=" << path;
+
+            if ((currentPermissions & std::filesystem::perms::others_write) != std::filesystem::perms::none) {
+                qCDebug(lcFileSystem()).nospace() << "removing others write permissions path=" << path;
+                std::filesystem::permissions(stdStrPath, std::filesystem::perms::others_write, std::filesystem::perm_options::remove);
+                permissionsDidChange = true;
+            }
+
+            if ((currentPermissions & std::filesystem::perms::owner_write) == std::filesystem::perms::none) {
+                qCDebug(lcFileSystem()).nospace() << "adding owner write permissions path=" << path;
+                std::filesystem::permissions(stdStrPath, std::filesystem::perms::owner_write, std::filesystem::perm_options::add);
+                permissionsDidChange = true;
+            }
+
             break;
         }
+        }
     }
     catch (const std::filesystem::filesystem_error &e)
     {
@@ -523,34 +559,16 @@ bool FileSystem::setFolderPermissions(const QString &path,
         return false;
     }
 
-    try
-    {
-        switch (permissions) {
-        case OCC::FileSystem::FolderPermissions::ReadOnly:
-            break;
-        case OCC::FileSystem::FolderPermissions::ReadWrite:
-            std::filesystem::permissions(stdStrPath, std::filesystem::perms::others_write, std::filesystem::perm_options::remove);
-            std::filesystem::permissions(stdStrPath, std::filesystem::perms::owner_write, std::filesystem::perm_options::add);
-            break;
-        }
-    }
-    catch (const std::filesystem::filesystem_error &e)
-    {
-        qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << e.path1().c_str() << e.path2().c_str();
-        return false;
-    }
-    catch (const std::system_error &e)
-    {
-        qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << "- path:" << stdStrPath;
-        return false;
-    }
-    catch (...)
-    {
-        qCWarning(lcFileSystem()) << "exception when modifying folder permissions -  path:" << stdStrPath;
-        return false;
+    if (permissionsDidChange) {
+        const auto newPermissions = std::filesystem::status(stdStrPath).permissions();
+        qCDebug(lcFileSystem()).nospace() << "updated permissions path=" << path << " perms=" << Qt::showbase << Qt::oct << static_cast<int>(newPermissions);
     }
 #endif
 
+    if (permissionsChanged) {
+        *permissionsChanged = permissionsDidChange;
+    }
+
     return true;
 }
 

diff --git a/src/libsync/filesystem.h b/src/libsync/filesystem.h
index 2682f180b84d5a2d36cd628232e1eb38d7b07560..f30a000a9aed2fcc5dd0566ea137a23caba10006 100644 (file)
--- a/src/libsync/filesystem.h
+++ b/src/libsync/filesystem.h
@@ -127,7 +127,8 @@ namespace FileSystem {
                                                const std::function<void(const QString &path, bool isDir)> &onError = nullptr);
 
     bool OWNCLOUDSYNC_EXPORT setFolderPermissions(const QString &path,
-                                                  FileSystem::FolderPermissions permissions) noexcept;
+                                                  FileSystem::FolderPermissions permissions,
+                                                  bool *permissionsChanged = nullptr) noexcept;
 
     bool OWNCLOUDSYNC_EXPORT isFolderReadOnly(const std::filesystem::path &path) noexcept;
 

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 2c66540f1c5eeaf06ef8a60d43c9df0808cdf753..b914caa79f4847fb44e77544e2c41210e6f8c097 100644 (file)
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -95,6 +95,8 @@ nextcloud_add_test(SyncConflictsModel)
 nextcloud_add_test(DateFieldBackend)
 nextcloud_add_test(ClientStatusReporting)
 
+nextcloud_add_test(FileSystem)
+
 nextcloud_add_test(FolderStatusModel)
 
 target_link_libraries(SecureFileDropTest PRIVATE Nextcloud::sync)

diff --git a/test/testfilesystem.cpp b/test/testfilesystem.cpp
new file mode 100644 (file)
index 0000000..50ba319
--- /dev/null
+++ b/test/testfilesystem.cpp
@@ -0,0 +1,133 @@
+/*
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: CC0-1.0
+ *
+ * This software is in the public domain, furnished "as is", without technical
+ * support, and with no warranty, express or implied, as to its usefulness for
+ * any purpose.
+ */
+
+#include <QtTest>
+#include <QTemporaryDir>
+
+#include "common/filesystembase.h"
+#include "logger.h"
+
+#include "libsync/filesystem.h"
+
+using namespace OCC;
+namespace std_fs = std::filesystem;
+
+class TestFileSystem : public QObject
+{
+    Q_OBJECT
+
+private:
+    QTemporaryDir testDir;
+
+private Q_SLOTS:
+    void initTestCase()
+    {
+        OCC::Logger::instance()->setLogFlush(true);
+        OCC::Logger::instance()->setLogDebug(true);
+
+        QStandardPaths::setTestModeEnabled(true);
+
+        QDir dir(testDir.path());
+        dir.mkdir("existingDirectory");
+    }
+
+#ifndef Q_OS_WIN
+    void testSetFolderPermissionsExistingDirectory_data()
+    {
+        constexpr auto perms_0555 =
+            std_fs::perms::owner_read | std_fs::perms::owner_exec
+            | std_fs::perms::group_read | std_fs::perms::group_exec
+            | std_fs::perms::others_read | std_fs::perms::others_exec;
+        constexpr auto perms_0755 = perms_0555 | std_fs::perms::owner_write;
+        constexpr auto perms_0775 = perms_0755 | std_fs::perms::group_write;
+
+        QTest::addColumn<std_fs::perms>("originalPermissions");
+        QTest::addColumn<FileSystem::FolderPermissions>("folderPermissions");
+        QTest::addColumn<bool>("expectedResult");
+        QTest::addColumn<bool>("expectedPermissionsChanged");
+        QTest::addColumn<std_fs::perms>("expectedPermissions");
+
+        QTest::newRow("0777, readonly -> 0555, changed")
+            << std_fs::perms::all
+            << FileSystem::FolderPermissions::ReadOnly
+            << true
+            << true
+            << perms_0555;
+
+        QTest::newRow("0555, readonly -> 0555, not changed")
+            << perms_0555
+            << FileSystem::FolderPermissions::ReadOnly
+            << true
+            << false
+            << perms_0555;
+
+        QTest::newRow("0777, readwrite -> 0775, changed")
+            << std_fs::perms::all
+            << FileSystem::FolderPermissions::ReadWrite
+            << true
+            << true
+            << perms_0775;
+
+        QTest::newRow("0775, readwrite -> 0775, not changed")
+            << perms_0775
+            << FileSystem::FolderPermissions::ReadWrite
+            << true
+            << false
+            << perms_0775;
+
+        QTest::newRow("0755, readwrite -> 0755, not changed")
+            << perms_0755
+            << FileSystem::FolderPermissions::ReadWrite
+            << true
+            << false
+            << perms_0755;
+
+        QTest::newRow("0555, readwrite -> 0755, changed")
+            << perms_0555
+            << FileSystem::FolderPermissions::ReadWrite
+            << true
+            << true
+            << perms_0755;
+    }
+
+    void testSetFolderPermissionsExistingDirectory()
+    {
+        QFETCH(std_fs::perms, originalPermissions);
+        QFETCH(FileSystem::FolderPermissions, folderPermissions);
+        QFETCH(bool, expectedResult);
+        QFETCH(bool, expectedPermissionsChanged);
+        QFETCH(std_fs::perms, expectedPermissions);
+
+        bool permissionsDidChange = false;
+        QString fullPath = testDir.filePath("existingDirectory");
+        const auto stdStrPath = fullPath.toStdWString();
+
+        std_fs::permissions(stdStrPath, originalPermissions);
+
+        QCOMPARE(FileSystem::setFolderPermissions(fullPath, folderPermissions, &permissionsDidChange), expectedResult);
+
+        const auto newPermissions = std_fs::status(stdStrPath).permissions();
+        QCOMPARE(newPermissions, expectedPermissions);
+        QCOMPARE(permissionsDidChange, expectedPermissionsChanged);
+    }
+
+    void testSetFolderPermissionsNonexistentDirectory()
+    {
+        bool permissionsDidChange = false;
+
+        QString fullPath = testDir.filePath("nonexistentDirectory");
+
+        QCOMPARE(FileSystem::setFolderPermissions("nonexistentDirectory", FileSystem::FolderPermissions::ReadOnly, &permissionsDidChange), false);
+        QCOMPARE(permissionsDidChange, false);
+    }
+#endif
+};
+
+QTEST_GUILESS_MAIN(TestFileSystem)
+#include "testfilesystem.moc"